-
Notifications
You must be signed in to change notification settings - Fork 125
Randomize chain source selection in tests #769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
|
This will be flaky, at the very least until lightningdevkit/rust-lightning#4341 gets release, which would have been caught if we'd ever had run the 0conf test case with an Electrum chain source. |
TheBlueMatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense to me. Hopefully CI fails until lightningdevkit/rust-lightning#4341
I think I'll whack-a-mole a few more flakes/bugs before landing this. |
|
Previously had failed, I want to double-check them once more before moving forward here. |
96b6f29 to
b8179ca
Compare
|
Pushed an update, but currently |
b8179ca to
282605b
Compare
It's weird to have a special intermediary `setup_node` method if we have `TestConfig` for exactly that reason by now. So we move `async_payment_role` over.
7796735 to
d0946d1
Compare
Alright, fixed that one, too and rebased. Should be ready for review now. |
.. all of our tests should be robust against switching chain sources. We here opt to pick a random one each time to considerably extend our test coverage, instead of just running some cases against non-Esplora chain sources. Signed-off-by: Elias Rohrer <[email protected]>
When we intially implemented `bitcoind` syncing polling the mempool was very frequent and rather inefficient so we made a choice not to unnecessarily update the payment store for mempool changes, especially since we only consider transactions `Succeeded` after `ANTI_REORG_DELAY` anyways. However, since then we made quite a few peformance improvements to the mempool syncing, and by now we should just update they payment store as not doing so will lead to rather unexpected behavior, making some tests fail for `TestChainSource::Bitcoind`, e.g., `channel_full_cycle_0conf`, which we fix here. As we recently switched to updating the payment store based on BDK's `WalletEvent`, but they currently don't offer an API returning such events when applying mempool transactions, we copy over the respective method for generating events from `bdk_wallet`, with the intention of dropping it again once they do. Signed-off-by: Elias Rohrer <[email protected]>
Previously, we fixed than a fresh node syncing via `bitcoind` RPC would resync all chain data back to genesis. However, while introducing a wallet birthday is great, it disallowed discovery of historical funds if a wallet would be imported from seed. Here, we add a recovery mode flag to the builder that explictly allows to re-enable resyncing from genesis in such a scenario. Going forward, we intend to reuse that API for an upcoming Lightning recoery flow, too.
Previously, we'd selectively insert the funding outputs into the onchain wallet to later allow calculating `fees_paid` when creating payment store entries (only for splicing mostly). However, this didn't always work, and we might for example end up with a missing funding output (and hence would fall back to `fees_paid: Some(0)`) if it was a counterparty-initiated channel and we synced via `bitcoind` RPC. Here, we fix this by tracking all LDK-registered `txids` in `ChainSource` and then in the `Wallet`'s `Listen` implementation insert all outputs of all registered transactions into the `Wallet`, ensuring we'd always have sufficient data for `calculate_fee` available. Thereby we also fix the `onchain_send_receive` test which previously failed when using `TestChainSource::Bitcoind`. Signed-off-by: Elias Rohrer <[email protected]>
d0946d1 to
7c08a0b
Compare
.. all of our tests should be robust against switching chain sources. We
here opt to pick a random one each time to considerably extend our test
coverage, instead of just running some cases against non-Esplora chain
sources.